-
Notifications
You must be signed in to change notification settings - Fork 721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New adapter: BoldwinX #3430
New adapter: BoldwinX #3430
Conversation
Code coverage summaryNote:
bwxRefer here for heat map coverage report
|
Code coverage summaryNote:
bwxRefer here for heat map coverage report
|
adapters/bwx/bwx.go
Outdated
bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(seats)) | ||
|
||
for _, seatBid := range seats { | ||
for bidId, bid := range seatBid.Bid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bidId
is actually index of loop iteration. Should consider renaming it to i or idx or index
to avoid confusion between bidId
and loop index
adapters/bwx/bwx.go
Outdated
default: | ||
return "", fmt.Errorf("failed to parse bid mtype for impression id \"%s\"", bid.ImpID) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should also include received mtype
in error message.
@@ -0,0 +1,19 @@ | |||
endpoint: "http://rtb.boldwin.live?pid={{.SourceId}}&host={{.Host}}&pbs=1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpoint is reachable
% curl -i --location --request POST 'http://rtb.boldwin.live?pid=12&host=1&pbs=1'
HTTP/1.1 400 Bad Request
@@ -0,0 +1,19 @@ | |||
endpoint: "http://rtb.boldwin.live?pid={{.SourceId}}&host={{.Host}}&pbs=1" | |||
maintainer: | |||
email: "[email protected]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prebid team has sent an email to verify specified maintainer's email address. Requesting to reply back on email with "received" message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
email verified.
Code coverage summaryNote:
bwxRefer here for heat map coverage report
|
@onkarvhanumante done, thank you for review |
adapters/bwx/bwx.go
Outdated
if bidderRawResponse.StatusCode == http.StatusServiceUnavailable { | ||
return nil, []error{&errortypes.BadInput{ | ||
Message: "Bidder BoldwinX is unavailable. Please contact the bidder support.", | ||
}} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be covered by CheckResponseStatusCodeErrors func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gargcreation1992 done
Code coverage summaryNote:
bwxRefer here for heat map coverage report
|
@SyntaxNode Hi Scott! Could you please submit requested changes? Thank you |
Hi @onkarvhanumante, Could you please let me know if there are any blockers to merge this PR? It would be great if bwx adapter could be included in the next release. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming "boldwinxtest" to "bwxtest" to follow the convention of using the adapter name.
adapters/bwx/bwxtest_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected file name is bwx_test.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this file to follow common design patterns.
adapters/bwx/bwxtest_test.go
Outdated
|
||
assert.NoError(t, buildErr) | ||
adapterstest.RunJSONBidderTest(t, "boldwinxtest", bidder) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a builder test for the macro parsing error. Here's an example from aceex:
func TestEndpointTemplateMalformed(t *testing.T) {
_, buildErr := Builder(openrtb_ext.BidderAceex, config.Adapter{
Endpoint: "{{Malformed}}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})
assert.Error(t, buildErr)
}
143844c
Code coverage summaryNote:
bwxRefer here for heat map coverage report
|
@SyntaxNode hello, thank you for review, now its done + dev-docs commit: prebid/prebid.github.io@4b70bcf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates. Just two minor nitpicks left.
adapters/bwx/bwxtest_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this file to follow common design patterns.
Code coverage summaryNote:
bwxRefer here for heat map coverage report
|
@SyntaxNode done |
Docs PR: prebid/prebid.github.io#5105